Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLN: Add/refine type hints to some functions in core.dtypes.cast #33286

Merged
merged 3 commits into from
Apr 5, 2020

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach commented Apr 4, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Some cleanup resulting from PR #32894. In order to get SeriesGroupBy._transform_fast to realize self.obj is a Series, it seemed I needed to add a generic type to GroupBy and _GroupBy. I don't know if there is a better way to do this. Here, one cannot use FrameOrSeries as a Series may return a DataFrame and vice-versa, so I added _GroupByT.

 - maybe_cast_result obj is now "Series"
 - Added type hint and assert to maybe_cast_to_extension_array
 - Added generic type to _GroupBy and GroupBy in core.groupby.groupby
 - Updated missed "try_cast" references to "maybe_cast"
@@ -329,6 +333,8 @@ def maybe_cast_to_extension_array(cls, obj, dtype=None):
ExtensionArray or obj
"""
assert isinstance(cls, type), f"must pass a type: {cls}"
assertion_msg = f"must pass a subclass of ExtensionArray: {cls}"
assert issubclass(cls, ABCExtensionArray), assertion_msg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the assert here still necessary, or does the type hint now make this redundant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonjayhawkins Would like your thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type hint is sufficient for the static checking (so that mypy doesn't report any errors). For the assert, it depends on the desired runtime behaviour.

@simonjayhawkins
Copy link
Member

In order to get SeriesGroupBy._transform_fast to realize self.obj is a Series, it seemed I needed to add a generic type to GroupBy and _GroupBy. I don't know if there is a better way to do this. Here, one cannot use FrameOrSeries as a Series may return a DataFrame and vice-versa, so I added _GroupByT.

see #32302 (comment)

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rhshadrach lgtm pending green

@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Apr 5, 2020
@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Apr 5, 2020
@jreback jreback merged commit b71e807 into pandas-dev:master Apr 5, 2020
@jreback
Copy link
Contributor

jreback commented Apr 5, 2020

thanks @rhshadrach very nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants